feat: rig/globals ambient call, pipeline null-skip, tsconfig baseUrl fix - #339
Conversation
…n, fix tsconfig baseUrl Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with minor suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Type-safety cast (
globals.ts:63):callImpl as unknown as WorkflowCallsilences the compiler; worth replacing with an explicit structural assignment. - Test coverage gaps:
call.text()/call.json()no-context paths are untested; the null-skip assertion doesn't pin which items were skipped.
Positive Highlights
- ✅ Clean opt-in design:
rig/globalsis fully additive — nothing is changed inrigcore. - ✅ The
pipelinenull-propagation fix is minimal, correct, and well-placed. - ✅ Good test coverage for the happy paths of all three
callvariants. - ✅
tsconfigcleanup (baseUrl removal + missing paths entries) is correct and long-overdue. - ✅ Documentation in
dynamic-workflows.mdclearly explains the new null-skip semantics.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 32.7 AIC · ⌖ 4.34 AIC · ⊞ 6.3K
Comment /matt to run again
| child: Workflow<Input, Output>, | ||
| args?: Input, | ||
| options?: WorkflowNestedOptions, | ||
| ): Promise<Output> => |
There was a problem hiding this comment.
[/codebase-design] The callImpl as unknown as WorkflowCall double-cast bypasses structural type checking — TypeScript cannot verify the shape is correct.
💡 Suggestion
Assign each member explicitly so TypeScript can enforce the WorkflowCall contract at the definition site:
export const call: WorkflowCall = Object.assign(callImpl, {
text: callImpl.text,
json: callImpl.json,
workflow: callImpl.workflow,
});This avoids the double cast while keeping the code just as concise.
| function requireContext(label: string): WorkflowCall { | ||
| const ctx = currentWorkflow(); | ||
| if (ctx === undefined) { | ||
| throw new Error(`${label} requires an active workflow run (call inside runWorkflow or a launcher program).`); |
There was a problem hiding this comment.
[/tdd] requireContext is called on every call.text(), call.json(), and call.workflow() invocation but is never tested in isolation — only the happy path via delegation is covered.
💡 Suggestion
Add a test for each sub-method throwing when called outside a run:
it('call.text() throws outside a workflow run', async () => {
await expect(ambientCall.text('ping')).rejects.toThrow('requires an active workflow run');
});
it('call.json() throws outside a workflow run', async () => {
await expect(ambientCall.json('ping', s.string)).rejects.toThrow('requires an active workflow run');
});Only call() is tested for the no-context path; the sub-methods could diverge silently.
| const stage2 = vi.fn((_prev: unknown, item: number) => item); | ||
| await expect( | ||
| pipeline([1, 2, 3], (_item: number) => _item === 2 ? null : _item * 10, stage2), | ||
| ).resolves.toEqual([1, null, 3]); |
There was a problem hiding this comment.
[/tdd] The pipeline null-skip test verifies that stage2 is called 2 times but does not assert which items it was called with — so a bug where it skips the wrong item would pass.
💡 Suggestion
Assert the exact calls to stage2:
expect(stage2).toHaveBeenCalledWith(10, 1, 0);
expect(stage2).toHaveBeenCalledWith(30, 3, 2);
expect(stage2).not.toHaveBeenCalledWith(expect.anything(), 2, expect.anything());This pins both the skipped item (index 1) and the values passed to the non-null stages.
callwas inaccessible at module scope without destructuring frombody;pipelinepassednullfrom a failed stage into the next stage;tsconfig.jsonemitted a TS5102 error for the removedbaseUrloption.rig/globals— opt-in ambient workflow primitivesNew entry point
"rig/globals"exposescall,pipeline, andparallelas ambient functions that route throughcurrentWorkflow()automatically. Nothing is added to"rig"— import explicitly to opt in.callmirrors the fullWorkflowCallsurface including.text(),.json(), and.workflow(). Throws with a clear message when no workflow run is active.pipeline— null propagation across stagesWhen stage N returns
null(agent failure), subsequent stages for that item are now skipped andnullpropagates to the output rather than being passed aspreviousto the next stage.tsconfig.jsonRemoved
baseUrl: "."(deprecated in TS 5.x, TS5102). The existingpathsconfig is sufficient.